fix(skippy): avoid duplicate lifecycle token handoff - #1202
Conversation
📝 WalkthroughWalkthrough
ChangesProposal generation-state validation
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
This pull request is currently a draft. Reviews will not take place until the PR is marked as ready for review. |
5679a89 to
9548567
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
crates/mesh-native-serving-plugin-host/src/lib.rs (1)
920-1044: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for committed counts below prompt counts.
Lines 472-475 reject this input before generation-state lookup. Add a proposal test with
committed_token_count < prompt_token_countand assert that the error contains"precedes prompt token count".Proposed test
+ #[test] + fn proposal_rejects_committed_count_before_prompt_count() { + let (active, _) = test_support::fake_active_with_events(Duration::ZERO); + let error = active + .propose( + LinearProposalQuery::new( + 1, + 2, + 2, + 1, + 1, + 8, + Instant::now() + Duration::from_millis(100), + ) + .with_pending_token_ids(vec![4].into_boxed_slice()), + ) + .unwrap_err(); + assert!(error.to_string().contains("precedes prompt token count")); + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/mesh-native-serving-plugin-host/src/lib.rs` around lines 920 - 1044, Add a test alongside the existing proposal validation tests that initializes a generation with a prompt token count higher than the query’s committed_token_count, calls Active::propose, and asserts the returned error contains "precedes prompt token count". Ensure the case exercises the early count validation before generation-state lookup.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@crates/mesh-native-serving-plugin-host/src/lib.rs`:
- Around line 920-1044: Add a test alongside the existing proposal validation
tests that initializes a generation with a prompt token count higher than the
query’s committed_token_count, calls Active::propose, and asserts the returned
error contains "precedes prompt token count". Ensure the case exercises the
early count validation before generation-state lookup.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 35691800-bf97-4497-bf18-f8d273077398
📒 Files selected for processing (1)
crates/mesh-native-serving-plugin-host/src/lib.rs
Problem
The public #1193 squash (
915e6918) introduced a regression where a request can carry pending tokens that were already delivered through the lifecyclecommit_generationpath. The proposal path then forwards those same tokens a second time. That duplicates the plugin's generated-token prefix and can make a valid proposal session fail closed.Fix
This is intentionally based on public
mainafter #1193 and contains no private Mesh architecture or Cacheline code.Validation
cargo fmt --all -- --checkcargo test -p mesh-native-serving-plugin-host --lib(24 passed)cargo clippy -p mesh-native-serving-plugin-host --lib --all-targets -- -D warningsThis draft PR only addresses duplicate proposal handoff state. Benchmark integration and session-header wiring are intentionally out of scope.